fix(sast-semgrep): fold the changed-scope gate into its single consumer job - #2147
Conversation
…er job One consumer, one runner: the standalone `changed-scope` job cost a second runner allocation per PR org-wide purely to compute two booleans for the `semgrep` job. The classifier now runs as a step inside `semgrep` (after harden-runner), the expensive steps gate on `steps.scope.outputs.code`, and the enforce step carries the same guard so a step-skipped scan's empty `rc` cannot fail a doc-only PR. The job keeps `if: github.event.action != 'closed'` with no needs-output term, so a doc-only run still concludes `success` (required-workflow-path-filter-boundary.md). strix.yml is left alone (hot-file collision zone). Measured in #1904; contract tests updated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The comment still said the classifier verdict was consumed through `needs`; nothing consumes it that way any more. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 47 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…gate-fold-20260913
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The fold of the changed-scope classifier into the single semgrep job is correct and safe. The classifier logic is unchanged (same step ID scope, same gh api logic), all expensive steps plus the final enforce step gate on steps.scope.outputs.code == 'true', and the job-level if: has no needs. term, so a doc-only PR still runs one job that concludes success. The enforce-step guard explicitly prevents the empty-rc false failure, and byte-level contract tests cover all five guard occurrences plus the exact enforce-step expression.
Reviewed changed lines
.github/workflows/sast-semgrep.yml:46 (RIGHT): Job-levelif:remainsgithub.event.action != 'closed'with noneeds.term, preserving the always-admitted job property required for doc-only PRs..github/workflows/sast-semgrep.yml:121 (RIGHT): The enforce step's guard includessteps.scope.outputs.code == 'true'before therc != '0'check, preventing a skipped Run Semgrep from failing on emptyrc..github/workflows/sast-semgrep.yml:215 (RIGHT): The complete conditionalways() && steps.scope.outputs.code == 'true' && (steps.semgrep_sarif.outputs.finding_count != '0' || steps.semgrep.outputs.rc != '0')short-circuits correctly, making the fold safe.tests/test_docs_only_pr_runner_admission.py:224 (RIGHT): Byte-level test asserts exactly 5steps.scope.outputs.code == 'true'occurrences and the exact enforce-step guard string, covering all gated steps.tests/test_required_security_runner_image_contract.py:31 (RIGHT): Runner-image contract updated to expect exactly oneruns-on: ubuntu-24.04, confirming no second job was reintroduced by the fold.
Adversarial validation
.github/workflows/sast-semgrep.yml:46 (RIGHT)falsified: A doc-only PR would leave the run skipped or fail due to emptyrcon the enforce step. — Job-levelif:at line 46 has noneeds.term, so the job runs; the enforce guard at line 215 includessteps.scope.outputs.code == 'true'before therccomparison, short-circuiting to false whencodeis false.tests/test_required_security_runner_image_contract.py:33 (RIGHT)falsified: A second runner job remains, costing an extra allocation per PR. — The contract test assertsworkflow.count("runs-on: ubuntu-24.04") == 1, and the diff shows exactly one job block with that image.tests/test_docs_only_pr_runner_admission.py:224 (RIGHT)falsified: Some expensive step is missing the classifier-output guard and would run on doc-only PRs. — The byte-level test asserts exactly 5 occurrences ofsteps.scope.outputs.code == 'true', covering checkout, verify-submitted-revision, verify-pinned-manifest, Run Semgrep, and Enforce-gate steps.- Residual risk: No residual risk identified; the fold is covered by byte-level contract tests and the job-level
if:preserves required-check behavior.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
aa52e2abfc5a9f4766c7088abdd4556f22178659 - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
Bypass-merge evidence (directive §11 infrastructure exception)Head merged: Change under review: partial #1976 — Local exact-head verification (detached worktree at this head): Non-passing required checks on this head at 01:05Z and their class:
Security/contract checks that did complete on this head all pass (Bandit, Semgrep, gitleaks, pip-audit, trivy-fs, scorecard, CodeRabbit, coverage where finished). The remaining blockers are the orchestrator/free review gateway (noema/opencode/strix, #2165/#2148/#1915) and runner-pool saturation (jobs queued for hours, #712/#1531); CodeQL "compatibility analysis" failures are the documented fail-closed placeholders ( Post-merge: the required review workflows re-run on 🤖 Generated with Claude Code |
Ordinary/non-force merge of protected main into the canonical #2079 branch. Preserves the bounded Noema repair and protected central advances; predecessor check/review freshness does not transfer.
Ordinary/non-force merge of protected main into the canonical #2170 branch. Preserves the coverage-RCA and full-suite dependency repair with current protected advances; predecessor check/review freshness does not transfer.
1 participant
What changed
sast-semgrep.ymlno longer has a standalonechanged-scopejob. The "Classify changed paths" step (byte-identical script) now runs inside the single consumersemgrep, right afterharden-runner(which must audit the classifier's owngh apiegress). The four expensive steps — checkout, revision verify, manifest verify,Run Semgrep— carryif: steps.scope.outputs.code == 'true', and so does the final "Enforce Semgrep gate" step. The job keepsif: github.event.action != 'closed'with noneeds.term and gainspull-requests: read(the permission the gate job carried).Measured in #1904 ("Items 15/16/17 measurement",
docs/product-technical-gap-baseline.md): the gate was one full runner allocation per PR spent computing two booleans for one downstream job. Net: one runner per PR for this workflow instead of two, org-wide (the workflow is ruleset-dispatched into ~74 repositories).Safety conditions, checked explicitly rather than assumed
18156473ignores everyon:filter, and.github's classic protection would leave a trigger-filtered context Pending forever (docs/doctoring/required-workflow-path-filter-boundary.md). Thesemgrepjob still runs on every PR and concludessuccesson a doc-only PR (steps skip, job succeeds).tests/test_docs_only_pr_runner_admission.py::test_each_gate_workflow_keeps_an_always_admitted_jobsemantics hold via the new dedicated test..githubclassic context disappears. NeitherDetect changed scopenorSemgrep (multi-language SAST)is among the 14 named contexts in.github's branch protection (list in the boundary doc), so nothing goes Pending there.always() && (... || steps.semgrep.outputs.rc != '0')evaluatesrcas the empty string whenRun Semgrepis step-skipped, and'' != '0'is true — every doc-only PR would have failed. The addedsteps.scope.outputs.code == 'true' &&on that step is what makes the fold safe.strix.yml, the other single-consumer gate, is deliberately untouched — it is a documented multi-PR hot-file collision zone.security-scan.yml's gate has four consumers and is legitimately amortized.Contract tests
tests/test_docs_only_pr_runner_admission.py:GATE_WORKFLOWSdropssast-semgrep.yml; the doc-pattern byte-identity check still includes it (thecaseline is unchanged); newtest_sast_semgrep_folds_the_gate_into_its_single_consumer_at_step_levelpins: nochanged-scope:job /needs, exactly oneruns-on, jobifexactly the close guard,pull-requests: read, five step guards, the enforce-step guard, harden-runner before the classifier.tests/test_required_security_runner_image_contract.py:runs-on: ubuntu-24.04count 2 → 1 with the reason in the docstring.docs/product-technical-gap-baseline.md: status line + dated paragraph under the Items 15/16/17 entry.Verification
coverage run -m pytest testson this head → 3040 passed, 1 skipped (macOS platform skip), 0 warnings; coverage 100%;interrogate100%;yaml.safe_loadsucceeds. Live confirmation that a doc-only PR'sSemgrep (multi-language SAST)check concludessuccessneeds this on protectedmain(the ruleset dispatchesmain's copy).🤖 Generated with Claude Code